merge upstream/master into oadp-dev - #30
Open
Joeavaikath wants to merge 41 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe change set updates backend path canonicalization, case-aware rule matching, user provisioning, upload enforcement, frontend accessibility and upload flows, security controls, repository documentation, and dependency update policy. ChangesBackend security and path handling
Frontend accessibility and upload behavior
Repository maintenance and documentation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… home
Proxy- and hook-authenticated users were auto-provisioned by applying the
default scope (".") and passing it straight to MakeUserDir, which normalizes
"." to "/". With CreateUserDir enabled, every provisioned user therefore
received the server root as its scope instead of a per-user home directory,
letting one user read, overwrite and delete another user's files.
The signup handler already cleared the scope before deriving the home
directory. Centralize that logic into Settings.CreateUserHome (clear the
scope when CreateUserDir is on and no explicit scope was supplied, derive the
home dir, then reject a scope already owned by another user) and use it from
signup, proxy and hook auth so the three provisioning paths cannot diverge.
Refs GHSA-j7jh-37pf-mf8h, GHSA-j2fc-28fx-hc8q
GetByScope compared scopes with a case-sensitive exact match, so on a case-insensitive filesystem two accounts whose scopes differ only in case (e.g. /users/CaseVictim and /users/casevictim) were treated as distinct even though both resolve to the same physical home directory. The second account could then read, overwrite and delete the first account's files. Match the scope case-insensitively so the collision check performed during signup, proxy and hook provisioning catches these aliases and rejects the duplicate. Refs GHSA-576v-w77m-gr84
The ?checksum= branch of the resource GET handler read the whole file to compute its digest and returned it without checking Perm.Download, unlike the sibling raw, preview, subtitle and X-Encoding paths. A user provisioned with Perm.Download=false could therefore obtain md5/sha1/sha256/sha512 digests of files they are not allowed to download, giving a file-content hash oracle, and force a full-file read. Return 202 Accepted before computing the checksum when the user lacks Perm.Download, matching the other read paths. This completes the fix for CVE-2026-35606, whose original patch gated only the content-returning paths. Refs GHSA-7whw-q6gh-xr59
The in-memory upload cache deleted expired incomplete uploads with a raw os.Remove on the absolute path it had stored, bypassing ScopedFs entirely. Because the stored path is only lexically cleaned (no symlink evaluation) and os.Remove resolves symlinked parent directories, a Create-only user could register an upload and then, within the 3-minute TTL, swap an in-scope ancestor directory for a symlink so the eviction deleted an arbitrary file outside their scope. Carry a removal callback with each cache entry and invoke it on eviction instead of os.Remove. For TUS uploads the callback deletes via the uploading user's scoped filesystem, whose Remove is guarded by the same within() check that CVE-2026-55667 added, so eviction can no longer follow a symlink out of scope. The redis backend ignores the callback (it never deleted partial files). Refs GHSA-m9f5-2232-frp6
The TUS PATCH handler copied the whole request body to disk and used the declared Upload-Length only to decide when the upload was complete. A client could declare a tiny Upload-Length and then stream an arbitrarily large body in a single PATCH, writing it all to disk, so any user with create permission could exhaust the filesystem. Bound each write to the bytes still expected (Upload-Length - Upload-Offset), reject a PATCH whose offset already exceeds the declared length, and roll back and reject a body that exceeds it. This complements the existing negative Upload-Length handling. Refs GHSA-ffv3-7h97-993q
…oning Unit tests for Settings.CreateUserHome (derives a per-user home, rejects a colliding scope, preserves an explicit scope) plus proxy and hook regression tests asserting that with CreateUserDir enabled two provisioned users receive distinct home directories instead of the server root. Refs GHSA-j7jh-37pf-mf8h, GHSA-j2fc-28fx-hc8q
The check that stops two usernames normalizing to the same home directory ran as a storage operation separate from the save, so two first-time users provisioned concurrently could both observe a free scope and both be saved into it, ending up sharing one home directory. Move the check into users.Storage.SaveProvisioned, which holds a single lock across the lookup and the save, and reduce CreateUserHome to deriving and creating the directory. This covers all three provisioning paths: signup, proxy auth and hook auth. Refs GHSA-j7jh-37pf-mf8h
Two usernames that normalize to the same home directory must not both be provisioned when their requests are handled concurrently. Also cover that an explicit, non-derived scope may still be legitimately shared. Refs GHSA-j7jh-37pf-mf8h
)" This reverts commit 032271b.
New config option: --branding.disableUserProfile This option allows to disable User Profile together with User settings. Signed-off-by: Michal Pryc <mpryc@redhat.com>
New config option: --branding.defaultLoginUser "username" This option allows to provide default Username, which will result in hiding username field from the welcome page. Signed-off-by: Michal Pryc <mpryc@redhat.com>
Removes Help options "Delete" and "Rename" if the following permissions are revoked from a particular user: --perm.delete=false --perm.rename=false Signed-off-by: Michal Pryc <mpryc@redhat.com>
Add multi-stage Containerfile for UBI-based builds. Signed-off-by: Michal Pryc <mpryc@redhat.com>
Adds OWNERS file. Signed-off-by: Michal Pryc <mpryc@redhat.com>
Fix downstream changes to match upstream refactored functions. Signed-off-by: Michal Pryc <mpryc@redhat.com>
Disable Validate Title check (semantic commit PR titles are not used downstream). Fix Go version to 1.25 and add oadp-* branches to CI triggers.
oadp-rebasebot-cloner
Bot
force-pushed
the
rebase-bot-oadp-dev
branch
from
July 27, 2026 01:02
fb611f6 to
e05c02e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts (1)
120-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStubbed globals aren't restored after the suite.
vi.stubGlobal("window", ...)/vi.stubGlobal("document", ...)are set inbeforeEachbut never unstubbed. Depending on test isolation config, this can leak the mockedwindow/documentinto other test files in the same worker.♻️ Suggested fix
beforeEach(() => { vi.clearAllMocks(); harness.fileStore.preselect = ""; vi.stubGlobal("window", { innerWidth: 1024, innerHeight: 768, addEventListener: vi.fn(), removeEventListener: vi.fn(), }); vi.stubGlobal("document", { getElementsByClassName: () => [], }); }); + + afterEach(() => { + vi.unstubAllGlobals(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts` around lines 120 - 132, Restore the globals stubbed in the upload conflict resolution test suite after each test by adding the appropriate Vitest unstubbing cleanup alongside the existing beforeEach setup. Ensure the mocked window and document do not leak into other test files while preserving the current per-test stubs.frontend/src/utils/upload.ts (1)
60-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefix match on
normBaselacks a path-boundary check.
path.startsWith(normBase)doesn't verify a separator follows the prefix, so a sibling directory whose name is a superstring of the target (e.g.target/My SubDir2/...vs. targettarget/My SubDir) will incorrectly match and get sliced with the wrong offset. Not covered by the new tests, which use non-overlapping names.♻️ Suggested boundary-safe fix
- const normBase = decodePath(removePrefix(basePath)).replace(/\/+$/, ""); + const normBase = decodePath(removePrefix(basePath)).replace(/\/+$/, "") + "/"; for (const entry of serverEntries) { const path = conflictPath(entry); const key = includeDirectories ? entry.name : path.startsWith(normBase) ? path.slice(normBase.length) : path;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/upload.ts` around lines 60 - 80, Update the prefix condition in buildConflictMap so normBase matches only when path equals the base or the next character is a path separator; otherwise use the full path unchanged. Preserve the existing slicing and key normalization behavior for valid boundary-aligned matches.settings/dir.go (1)
44-69: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚖️ Poor tradeoffDirectory is created before the scope-collision check runs.
CreateUserHomecallsMakeUserDir, which doesfs.MkdirAllimmediately, before any caller invokesusers.Storage.SaveProvisioned(the actual collision check + persist). IfSaveProvisionedlater rejects the user withErrExist, the directory that was already created on disk is never cleaned up. On a case-insensitive filesystem this is harmless (the "colliding" scope is the same physical directory as the winning user's), but on a case-sensitive filesystem it leaves an orphaned empty directory, and since this path is reachable from the public/api/signupendpoint (when signup is enabled), it's a small avenue for disk litter from failed provisioning attempts.Not blocking given the bounded impact, but worth considering: compute the candidate scope path without creating the directory, run the collision check, and only
MkdirAllafterSaveProvisionedsucceeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@settings/dir.go` around lines 44 - 69, Refactor CreateUserHome and the provisioning flow so candidate scope paths are computed without creating directories, allowing users.Storage.SaveProvisioned to perform collision validation first. Only call MakeUserDir or otherwise create the directory after SaveProvisioned succeeds, while preserving derived-scope handling and the returned user.Scope value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@auth/hook.go`:
- Around line 160-169: Update the explicit scope detection in the provisioning
flow around a.Fields.Values["user.scope"] so explicitScope is true only when the
hook-provided value is non-empty. Preserve automatic scope derivation and ensure
the resulting SaveProvisioned call performs the concurrency-safe collision check
when user.scope= is empty.
In `@files/case.go`:
- Around line 22-24: Update the TempFile error fallback in the case-sensitivity
probing logic to fail closed: treat probe failure as case-insensitive by
default, while preserving any reliable mount-specific determination already
available. Do not return a Linux-specific case-sensitive result when the probe
cannot be created.
In `@frontend/src/components/files/ExtendedImage.vue`:
- Around line 60-62: Update the image preview watch handler around
imgex.value.alt to guard decoding of the final src path segment, matching the
existing decodePath safeguard behavior: malformed percent-encoding must not
throw, and the malformed segment should remain unchanged while empty segments
still use "preview". Apply the same fix to both affected occurrences.
In `@http/tus_handlers.go`:
- Around line 112-118: Update the Redis-backed implementation of cache.Register
so it preserves the upload cleanup callback or equivalent scoped cleanup
metadata alongside the cached upload entry. Ensure Redis TTL expiration triggers
cleanup of the abandoned file through the registered callback, while retaining
the user-scoped d.user.Fs.Remove(uploadPath) behavior from the PATCH handler and
avoiding deletion outside the scoped filesystem.
In `@http/upload_cache_redis.go`:
- Around line 42-44: Update redisUploadCache.Register to retain and execute the
scoped remove callback when the Redis entry expires, ensuring both metadata and
the associated partial file are cleaned up. Persist sufficient cleanup metadata
or route it through the existing shared expiry mechanism, and make cleanup safe
and idempotent.
In `@SECURITY.md`:
- Around line 23-24: Update the SECURITY.md vulnerability-reporting guidance to
route all suspected vulnerabilities through the private Security page during
triage. Remove the severity-based public issue instruction, or replace it with
precise criteria explicitly defining when public disclosure is safe.
---
Nitpick comments:
In `@frontend/src/utils/upload.ts`:
- Around line 60-80: Update the prefix condition in buildConflictMap so normBase
matches only when path equals the base or the next character is a path
separator; otherwise use the full path unchanged. Preserve the existing slicing
and key normalization behavior for valid boundary-aligned matches.
In `@frontend/src/views/files/__tests__/upload-conflict-resolution.test.ts`:
- Around line 120-132: Restore the globals stubbed in the upload conflict
resolution test suite after each test by adding the appropriate Vitest
unstubbing cleanup alongside the existing beforeEach setup. Ensure the mocked
window and document do not leak into other test files while preserving the
current per-test stubs.
In `@settings/dir.go`:
- Around line 44-69: Refactor CreateUserHome and the provisioning flow so
candidate scope paths are computed without creating directories, allowing
users.Storage.SaveProvisioned to perform collision validation first. Only call
MakeUserDir or otherwise create the directory after SaveProvisioned succeeds,
while preserving derived-scope handling and the returned user.Scope value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 596f46f2-ccb8-40b9-9717-274fbf1ecd00
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (53)
.claude/CLAUDE.mdCHANGELOG.mdREADME.mdSECURITY.mdauth/hook.goauth/hook_test.goauth/proxy.goauth/proxy_test.gocmd/cmd_test.gocmd/config.gofiles/case.gofiles/case_test.gofrontend/public/index.htmlfrontend/src/App.vuefrontend/src/components/files/ExtendedImage.vuefrontend/src/components/files/ListingItem.vuefrontend/src/components/header/HeaderBar.vuefrontend/src/i18n/de.jsonfrontend/src/router/index.tsfrontend/src/utils/__tests__/check-conflict.test.tsfrontend/src/utils/upload.tsfrontend/src/views/Share.vuefrontend/src/views/files/FileListing.vuefrontend/src/views/files/__tests__/upload-conflict-resolution.test.tsgo.modhttp/auth.gohttp/data.gohttp/http.gohttp/preview.gohttp/public.gohttp/raw.gohttp/resource.gohttp/resource_checksum_test.gohttp/rules_path_test.gohttp/static.gohttp/tus_handlers.gohttp/tus_upload_length_test.gohttp/upload_cache_memory.gohttp/upload_cache_memory_test.gohttp/upload_cache_redis.gohttp/utils.gohttp/utils_test.gorenovate.jsonrules/rules.gorules/rules_test.gosettings/dir.gosettings/dir_test.gosettings/settings.gostorage/bolt/users.gostorage/bolt/users_test.gousers/storage.gousers/storage_test.gowww/docs/installation.md
💤 Files with no reviewable changes (1)
- http/raw.go
Comment on lines
+160
to
+169
| // A scope explicitly returned by the hook takes precedence over the | ||
| // automatic per-user home directory derivation. | ||
| _, explicitScope := a.Fields.Values["user.scope"] | ||
| derivedScope, err := a.Settings.CreateUserHome(u, a.Server.Root, explicitScope) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("user: failed to mkdir user home dir: [%s]", userHome) | ||
| return nil, err | ||
| } | ||
| u.Scope = userHome | ||
| log.Printf("user: %s, home dir: [%s].", u.Username, userHome) | ||
| log.Printf("user: %s, home dir: [%s].", u.Username, u.Scope) | ||
|
|
||
| err = a.Users.Save(u) | ||
| if err != nil { | ||
| if err := a.Users.SaveProvisioned(u, derivedScope); err != nil { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching hook.go:\n'
fd -a 'hook\.go$' . || true
printf '\nRelevant auth/hook.go excerpt:\n'
sed -n '1,240p' auth/hook.go
printf '\nSearch CreateUserHome/SaveProvisioned/MakeUserDir definitions:\n'
rg -n "func .*CreateUserHome|func .*SaveProvisioned|func .*MakeUserDir|type .*Fields|GetString\\(" -S .Repository: migtools/filebrowser
Length of output: 12609
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'auth/hook.go hookFields implementation:\n'
sed -n '230,310p' auth/hook.go
printf '\nsettings/dir.go CreateUserHome/MakeUserDir implementation:\n'
sed -n '1,120p' settings/dir.go
printf '\nusers/storage.go SaveProvisioned implementation:\n'
sed -n '80,170p' users/storage.go
printf '\nsettings dir tests around CreateUserHome:\n'
sed -n '1,120p' settings/dir_test.go
printf '\nfield string parsing probe for present/empty user.scope:\n'
python3 - <<'PY'
def getValues(s):
m={}
for line in s.replace('\r\n','\n').splitlines():
v=line.split('=',1)
if len(v)==2:
k=fv=v[0].strip(), v[1].strip()
m[k]=fv
return m
for s in ['user.scope=\nhook.action=auth\n','user.scope=/home/alice\nhook.action=auth\n','hook.action=auth\n']:
print(repr(s), getValues(s), "has user.scope:", "user.scope" in getValues(s).keys(), "value:", repr(getValues(s).get("user.scope","<missing>")))
PYRepository: migtools/filebrowser
Length of output: 8538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'auth/hook.go hookFields implementation:\n'
sed -n '230,310p' auth/hook.go
printf '\nsettings/dir.go CreateUserHome/MakeUserDir implementation:\n'
sed -n '1,120p' settings/dir.go
printf '\nusers/storage.goSaveProvisioned implementation:\n'
sed -n '80,170p' users/storage.go
printf '\nsettings dir tests around CreateUserHome:\n'
sed -n '1,120p' settings/dir_test.go
printf '\nfield string parsing probe for present/empty usser.scope:\n'
python3 - <<'PY'
def getValues(s):
m={}
for line in s.replace('\r\n','\n').splitlines():
v=line.split('=',1)
if len(v)==2:
m[v[0].strip()]=v[1].strip()
return m
for s in ['user.scope=\nhook.action=auth\n','user.scope=/home/alice\nhook.action=auth\n','hook.action=auth\n']:
print(repr(s), getValues(s), "has user.scope:", "user.scope" in getValues(s), "value:", repr(getValues(s).get("user.scope", "<missing>")))
PYRepository: migtools/filebrowser
Length of output: 8393
Require a non-empty value for hook-provided explicit scopes.
user.scope= is parsed as present with value ""; GetUser then uses that empty value, while explicitScope := true keeps CreateUserHome from reporting a derived scope. With CreateUserDir enabled, MakeUserDir still auto-derives a per-username home, but SaveProvisioned(u, false) skips the concurrency-safe scope collision check. Treat the scope as explicit only when the hook supplied a non-empty value.
🔒 Proposed fix
- // A scope explicitly returned by the hook takes precedence over the
- // automatic per-user home directory derivation.
- _, explicitScope := a.Fields.Values["user.scope"]
+ // A scope explicitly returned by the hook takes precedence over the
+ //automatic per-user home directory derivation. Treat the scope as
+ // explicit only when the hook actually supplied a non-empty value —
+ // an empty value must still go through automatic per-user derivation
+ // and its collision check.
+ hookScope, hasHookScope := a.Fields.Values["user.scope"]
+ explicitScope := hasHookScope && hookScope != ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // A scope explicitly returned by the hook takes precedence over the | |
| // automatic per-user home directory derivation. | |
| _, explicitScope := a.Fields.Values["user.scope"] | |
| derivedScope, err := a.Settings.CreateUserHome(u, a.Server.Root, explicitScope) | |
| if err != nil { | |
| return nil, fmt.Errorf("user: failed to mkdir user home dir: [%s]", userHome) | |
| return nil, err | |
| } | |
| u.Scope = userHome | |
| log.Printf("user: %s, home dir: [%s].", u.Username, userHome) | |
| log.Printf("user: %s, home dir: [%s].", u.Username, u.Scope) | |
| err = a.Users.Save(u) | |
| if err != nil { | |
| if err := a.Users.SaveProvisioned(u, derivedScope); err != nil { | |
| // A scope explicitly returned by the hook takes precedence over the | |
| // automatic per-user home directory derivation. Treat the scope as | |
| // explicit only when the hook actually supplied a non-empty value — | |
| // an empty value must still go through automatic per-user derivation | |
| // and its collision check. | |
| hookScope, hasHookScope := a.Fields.Values["user.scope"] | |
| explicitScope := hasHookScope && hookScope != "" | |
| derivedScope, err := a.Settings.CreateUserHome(u, a.Server.Root, explicitScope) | |
| if err != nil { | |
| return nil, err | |
| } | |
| log.Printf("user: %s, home dir: [%s].", u.Username, u.Scope) | |
| if err := a.Users.SaveProvisioned(u, derivedScope); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@auth/hook.go` around lines 160 - 169, Update the explicit scope detection in
the provisioning flow around a.Fields.Values["user.scope"] so explicitScope is
true only when the hook-provided value is non-empty. Preserve automatic scope
derivation and ensure the resulting SaveProvisioned call performs the
concurrency-safe collision check when user.scope= is empty.
Comment on lines
+22
to
+24
| probe, err := afero.TempFile(fs, root, "fb-case-probe-") | ||
| if err != nil { | ||
| return runtime.GOOS == "windows" || runtime.GOOS == "darwin" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when case-sensitivity probing is unavailable.
A read-only case-insensitive Linux mount (for example, CIFS) reaches this fallback and returns false, allowing case-variant paths to evade rules. Treat probe failure as case-insensitive unless a reliable mount-specific determination is available.
Proposed fix
if err != nil {
- return runtime.GOOS == "windows" || runtime.GOOS == "darwin"
+ return true
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| probe, err := afero.TempFile(fs, root, "fb-case-probe-") | |
| if err != nil { | |
| return runtime.GOOS == "windows" || runtime.GOOS == "darwin" | |
| probe, err := afero.TempFile(fs, root, "fb-case-probe-") | |
| if err != nil { | |
| return true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@files/case.go` around lines 22 - 24, Update the TempFile error fallback in
the case-sensitivity probing logic to fail closed: treat probe failure as
case-insensitive by default, while preserving any reliable mount-specific
determination already available. Do not return a Linux-specific case-sensitive
result when the probe cannot be created.
Comment on lines
+60
to
+62
| imgex.value.alt = decodeURIComponent( | ||
| props.src.split("/").pop() || "preview" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded decodeURIComponent can throw and break image preview.
If the last path segment contains malformed percent-encoding, decodeURIComponent throws synchronously, and the "preview" fallback only covers an empty segment, not a decode error. This mirrors exactly the malformed-segment scenario the new decodePath helper in frontend/src/utils/upload.ts was added to guard against (see its try/catch and the "leaves malformed destination path segments unchanged" test), but this file doesn't apply the same safeguard.
🛡️ Proposed fix
+function safeDecode(segment: string): string {
+ try {
+ return decodeURIComponent(segment);
+ } catch {
+ return segment;
+ }
+}
+
onMounted(() => {
if (!decodeUTIF() && imgex.value !== null) {
imgex.value.src = props.src;
- imgex.value.alt = decodeURIComponent(
- props.src.split("/").pop() || "preview"
- );
+ imgex.value.alt = safeDecode(props.src.split("/").pop() || "preview");
}Apply the same change in the watch handler.
Also applies to: 94-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/files/ExtendedImage.vue` around lines 60 - 62, Update
the image preview watch handler around imgex.value.alt to guard decoding of the
final src path segment, matching the existing decodePath safeguard behavior:
malformed percent-encoding must not throw, and the malformed segment should
remain unchanged while empty segments still use "preview". Apply the same fix to
both affected occurrences.
Comment on lines
+112
to
+118
| // Enables the user to utilize the PATCH endpoint for uploading file data. | ||
| // The removal callback deletes an abandoned upload through the user's | ||
| // scoped filesystem, so eviction cannot follow a symlink out of scope. | ||
| uploadPath := r.URL.Path | ||
| cache.Register(file.RealPath(), uploadLength, func() error { | ||
| return d.user.Fs.Remove(uploadPath) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Implement abandoned-upload cleanup for Redis-backed uploads.
http/upload_cache_redis.go:44-49 discards this callback, so Redis TTL expiry removes only metadata while partial files remain in user storage. Persist enough scoped cleanup metadata and delete abandoned files through a Redis-compatible expiry/cleanup mechanism.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/tus_handlers.go` around lines 112 - 118, Update the Redis-backed
implementation of cache.Register so it preserves the upload cleanup callback or
equivalent scoped cleanup metadata alongside the cached upload entry. Ensure
Redis TTL expiration triggers cleanup of the abandoned file through the
registered callback, while retaining the user-scoped
d.user.Fs.Remove(uploadPath) behavior from the PATCH handler and avoiding
deletion outside the scoped filesystem.
Comment on lines
+42
to
+44
| // Register stores the upload length. The scoped removal callback is unused by | ||
| // the redis backend, which does not delete partial files on eviction. | ||
| func (c *redisUploadCache) Register(filePath string, fileSize int64, _ func() error) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Implement expiry cleanup for the Redis backend.
Discarding remove means Redis TTL expiry only deletes metadata; abandoned files remain on disk. Persist enough scoped cleanup metadata and process it safely, or provide an equivalent shared cleanup worker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@http/upload_cache_redis.go` around lines 42 - 44, Update
redisUploadCache.Register to retain and execute the scoped remove callback when
the Redis entry expires, ensuring both metadata and the associated partial file
are cleaned up. Persist sufficient cleanup metadata or route it through the
existing shared expiry mechanism, and make cleanup safe and idempotent.
Comment on lines
+23
to
+24
| - **Critical:** report privately via the [Security](https://github.com/filebrowser/filebrowser/security) page. | ||
| - **Non-critical:** open a public issue so the community can help; we'll label it as a security issue. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep suspected vulnerability reports private until triage.
Directing “non-critical” vulnerabilities to public issues can disclose exploitable details before the project confirms impact or ships a fix, and “critical” is not defined here. Route all suspected vulnerabilities through the private Security channel, or document precise criteria for cases safe to disclose publicly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY.md` around lines 23 - 24, Update the SECURITY.md
vulnerability-reporting guidance to route all suspected vulnerabilities through
the private Security page during triage. Remove the severity-based public issue
instruction, or replace it with precise criteria explicitly defining when public
disclosure is safe.
Author
|
/retest |
|
@Joeavaikath: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated rebase PR generated by RebaseBot.
Summary
https://github.com/filebrowser/filebrowser:masterhttps://github.com/migtools/filebrowser:oadp-devLogs
View job log
Summary by CodeRabbit
Bug Fixes
Accessibility
Documentation